This is a comment left during a code review.
Path: rust/cymbal/src/frames/mod.rs
Line: 116-118

Comment:
**`warn!` log level for expected failures will be very noisy**

`metric_reason()` is called twice on the same value — once for the log and once for the counter. Since it returns `&'static str` and is pure, this is a minor inefficiency. Worth binding to a local variable to make the OnceAndOnlyOnce principle explicit.

Additionally, the `warn!` level fires for _all_ resolution failures, including very common expected cases like `no_symbol_set` (user never uploaded symbols) and `no_reference` (frame has no source URL). In a high-traffic system this will produce a flood of `WARN` log entries that obscure genuinely unexpected failures (e.g. `network_error` or `invalid_data`). Consider logging at `info` for expected cases and reserving `warn` for genuinely unexpected categories:

```suggestion
                } else if let Some(err) = &frame.resolve_failure {
                    let reason = err.metric_reason();
                    match reason {
                        "network_error" | "invalid_data" => {
                            tracing::warn!(lang = lang_tag, reason = reason, error = %err, "frame resolution failed");
                        }
                        _ => {
                            tracing::debug!(lang = lang_tag, reason = reason, error = %err, "frame resolution failed");
                        }
                    }
                    metrics::counter!(FRAME_NOT_RESOLVED, "lang" => lang_tag, "reason" => reason)
                        .increment(1);
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: rust/cymbal/src/frames/mod.rs
Line: 424-431

Comment:
**`serialize` has an unreachable `None` branch**

Because the field is annotated with `skip_serializing_if = "Option::is_none"`, serde will only invoke `serialize_with` when the value is `Some(...)`. The `None => serializer.serialize_none()` arm is therefore dead code and can mislead future readers into thinking the function could be called with `None`.

```suggestion
    pub fn serialize<S>(value: &Option<FrameError>, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        // `skip_serializing_if = "Option::is_none"` guarantees value is Some here,
        // but we match exhaustively to satisfy the type checker.
        match value {
            Some(err) => serializer.serialize_str(&err.to_string()),
            None => unreachable!("skip_serializing_if = Option::is_none prevents None reaching here"),
        }
    }
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: rust/cymbal/src/frames/mod.rs
Line: 434-442

Comment:
**Lossy deserialization silently drops `resolve_failure` data**

The comment says "production never deserializes `Frame`", but the `FrameError` enum itself is [documented to be serialized/deserialized for PG caching](rust/cymbal/src/error.rs#L64-L66). If that assumption ever changes, or if a test round-trips a resolved `Frame` and checks `resolve_failure`, the silent `None` return could mask bugs.

At minimum, the assumption should be verified at compile time (e.g. by removing `Deserialize` from `Frame` entirely) or the comment should be strengthened to explain *why* `Frame` will never be deserialized in production (e.g., only `RawFrame` is deserialized from Kafka and `Frame` is only written to ClickHouse/PG for querying, never read back as a `Frame`).

How can I resolve this? If you propose a fix, please make it concise.